-
Notifications
You must be signed in to change notification settings - Fork 42
Http framework integration refactor WIP #221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors HTTP hosting code by extracting common CloudAdapter and ChannelService functionality into a shared base implementation in the core library. The changes eliminate code duplication between FastAPI and aiohttp hosting adapters by introducing framework-agnostic base classes.
Key changes:
- Introduced
CloudAdapterBasein core with abstract methods for framework-specific behavior - Added
ChannelServiceOperationsclass to centralize channel API operations - Consolidated serialization/deserialization logic with
parse_agents_modelandserialize_agents_modelutilities
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/http.py | New file containing shared HTTP utilities, base classes, and operations |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/init.py | Exports new shared HTTP components |
| libraries/microsoft-agents-hosting-fastapi/microsoft_agents/hosting/fastapi/cloud_adapter.py | Refactored to extend CloudAdapterBase and implement framework-specific methods |
| libraries/microsoft-agents-hosting-fastapi/microsoft_agents/hosting/fastapi/channel_service_route_table.py | Simplified to use shared ChannelServiceOperations and serialization utilities |
| libraries/microsoft-agents-hosting-fastapi/microsoft_agents/hosting/fastapi/agent_http_adapter.py | Simplified to alias the shared AgentHttpAdapterProtocol |
| libraries/microsoft-agents-hosting-fastapi/microsoft_agents/hosting/fastapi/_start_agent_process.py | Delegates to shared core implementation |
| libraries/microsoft-agents-hosting-aiohttp/microsoft_agents/hosting/aiohttp/cloud_adapter.py | Refactored to extend CloudAdapterBase and implement framework-specific methods |
| libraries/microsoft-agents-hosting-aiohttp/microsoft_agents/hosting/aiohttp/channel_service_route_table.py | Simplified to use shared ChannelServiceOperations and serialization utilities |
| libraries/microsoft-agents-hosting-aiohttp/microsoft_agents/hosting/aiohttp/agent_http_adapter.py | Simplified to alias the shared AgentHttpAdapterProtocol |
| libraries/microsoft-agents-hosting-aiohttp/microsoft_agents/hosting/aiohttp/_start_agent_process.py | Delegates to shared core implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise HTTPException(status_code=415, detail="Unsupported Media Type") | ||
| def _get_claims_identity(self, request: Request) -> ClaimsIdentity | None: | ||
| return getattr( | ||
| request.state, "claims_identity", self._default_claims_identity() |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _get_claims_identity method returns a default ClaimsIdentity when 'claims_identity' is not found on request.state, but the base class process method expects None to be returned when no claims identity exists (lines 293-295 in http.py check if not claims_identity). This causes the default identity to be created twice - once here and once in the base class - which is inconsistent with the aiohttp implementation and the base class contract. The method should return None instead of self._default_claims_identity() as the default, or return getattr(request.state, 'claims_identity', None).
| request.state, "claims_identity", self._default_claims_identity() | |
| request.state, "claims_identity", None |
| else: | ||
| raise HTTPUnsupportedMediaType() | ||
| def _get_claims_identity(self, request: Request) -> ClaimsIdentity | None: | ||
| return request.get("claims_identity", self._default_claims_identity()) |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _get_claims_identity method returns a default ClaimsIdentity when 'claims_identity' is not found in the request, but the base class process method expects None to be returned when no claims identity exists (lines 293-295 in http.py check if not claims_identity). This causes the default identity to be created twice - once here and once in the base class. The method should return None as the default instead of self._default_claims_identity(), or return request.get('claims_identity', None).
| return request.get("claims_identity", self._default_claims_identity()) | |
| return request.get("claims_identity", None) |
| # Licensed under the MIT License. | ||
| from traceback import format_exc | ||
| from typing import Optional | ||
| from typing import Any, Optional |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Optional' is not used.
| from typing import Any, Optional | |
| from typing import Any |
| # Licensed under the MIT License. | ||
| from traceback import format_exc | ||
| from typing import Optional | ||
| from typing import Any, Optional |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Optional' is not used.
| from typing import Any, Optional | |
| from typing import Any |
This pull request refactors the aiohttp hosting layer to better leverage shared abstractions from the core library, resulting in a cleaner, more maintainable codebase. The main changes involve replacing framework-specific implementations with reusable protocols and base classes, centralizing request processing logic, and simplifying serialization and deserialization of models.
Refactoring to use shared core abstractions:
agent_http_adapter.py: Replaces the customAgentHttpAdapterprotocol with an alias for the sharedAgentHttpAdapterProtocolfrom core, removing unnecessary boilerplate and abstract methods.cloud_adapter.py: RefactorsCloudAdapterto inherit fromCloudAdapterBaseand implements required hooks for request handling, error responses, and claims identity extraction, removing redundant logic and error handling.Centralizing and simplifying request handling:
_start_agent_process.py: Replaces local validation and processing logic instart_agent_processwith a direct call to the sharedstart_agent_processfrom core, streamlining agent startup. [1] [2]Improving route table and serialization logic:
channel_service_route_table.py: Refactors route handlers to use sharedChannelServiceOperationsfor business logic, and introduces_read_payloadand_json_responsehelpers for consistent payload handling and serialization. Removes manual model validation and custom serialization code.Expanding core exports:
__init__.py(core): Adds new shared abstractions and helpers to the public API, including protocols, operations, serialization utilities, and the agent process starter. [1] [2]